-
Notifications
You must be signed in to change notification settings - Fork 26
Add all messages list to ServicePulse #2275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major, just a few comments that can be looked at in subsequent PRs
<div> | ||
<OnOffSwitch :id="id" @toggle="toggleRefresh" :value="autoRefresh" /> | ||
</div> | ||
<input type="number" v-model="refreshTimeout" min="1" max="600" v-on:change="updateTimeout" /> | ||
<span class="unit">s</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a dropdown with pre-set refresh ratings including zero be better option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works, and looks fine...
<i class="fa fa-lg fa-refresh" /> | ||
</button> | ||
<span>|</span> | ||
<label>Auto-Refresh:</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some kind of visual indication that we are fetching new data - https://fontawesome.com/icons/spinner?f=classic&s=solid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as idea
alias?: string; | ||
redirect?: string; | ||
title: string; | ||
component: RouteComponent | (() => Promise<RouteComponent>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it may be better to introduce a new type, e.g:
export interface RedirectRouteItem {
path: string;
title: string;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type reflects what VueRouter expects
|
||
const dataRetriever = useAutoRefresh(async () => { | ||
try { | ||
const [response, data] = await useTypedFetchFromServiceControl<Message[]>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be good to introduce a typed client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't understand what you mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at throughputclient
function friendlyTypeName(messageType: string) { | ||
if (messageType == null) return null; | ||
const typeClass = messageType.split(",")[0]; | ||
const typeName = typeClass.split(".").reverse()[0]; | ||
return typeName.replace(/\+/g, "."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be problematic, it may be worth adding a card to the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other places, we show the whole type name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added card
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that it may also make sense to save in local storage the current setting, so it won't reset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as idea
|
||
<template> | ||
<div class="refresh-config"> | ||
<button class="fa" title="refresh" @click="() => emit('manualRefresh')"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<button class="fa" title="refresh" @click="() => emit('manualRefresh')"> | |
<button class="btn btn-secondary btn-sm" title="refresh" @click="() => emit('manualRefresh')"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I don't want this to look like other buttons in the system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this button be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of the layout
Co-authored-by: John Simons <[email protected]>
# Conflicts: # src/Frontend/src/components/failedmessages/MessageRedirectForBackwardsCompatibility.vue
In this PR